Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ssa): Deduplicate intrinsics with predicates #6615

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 25, 2024

Description

Problem*

Resolves #6533

Summary*

Allows Instruction::Intrinsic types to be deduplicated in constant_folding if the Context is in use_constraint_info mode, and the Intrinsic is one that doesn't have a side effect other than the possibility of violating an implicit constraint on its inputs, which should be preserved in the first occurrence.

Additional Context

I found it a bit confusing why we're using use_constraint_info for these whenrequires_acir_gen_predicate returns false for all these instructions, which will cause cached_instruction_results to contain None for the predicate regardless of use_constraint_info. For some, but not all intrinsics, the predicate that tells whether the side effect is enabled is attached to their inputs during Ssa::flatten_cfg, and they don't depend any more on the enable_side_effect instruction. But it seems like this flag is the one that communicates whether we want such instructions to be deduplicated, so I used it even though I'm not sure what adverse effect we would see if we just returned true for these, other than the implicit coupling by knowing that flatten_cfg has done things in a certain way.

Another method that seems to have some of the truth is remove_enable_side_effects::Context::responds_to_side_effects_var

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Nov 25, 2024

Changes to number of Brillig opcodes executed

Generated at commit: ad51b7cfbdd0d011f29bd325234798d8793f56ba, compared to commit: 10a9f8104e1ebb8fad044927ff130a0e2ce9131b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
slice_regex +44 ❌ +1.29%
merkle_insert -33 ✅ -0.87%

Full diff report 👇
Program Brillig opcodes (+/-) %
slice_regex 3,442 (+44) +1.29%
sha256_regression 116,198 (-6) -0.01%
eddsa 735,024 (-3,594) -0.49%
merkle_insert 3,757 (-33) -0.87%

Copy link
Contributor

github-actions bot commented Nov 25, 2024

Changes to Brillig bytecode sizes

Generated at commit: ad51b7cfbdd0d011f29bd325234798d8793f56ba, compared to commit: 10a9f8104e1ebb8fad044927ff130a0e2ce9131b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
merkle_insert -11 ✅ -1.42%
to_le_bytes -10 ✅ -7.52%

Full diff report 👇
Program Brillig opcodes (+/-) %
sha256_regression 6,562 (-6) -0.09%
eddsa 10,665 (-22) -0.21%
merkle_insert 764 (-11) -1.42%
to_le_bytes 123 (-10) -7.52%

Copy link
Contributor

github-actions bot commented Nov 25, 2024

Changes to circuit sizes

Generated at commit: ad51b7cfbdd0d011f29bd325234798d8793f56ba, compared to commit: 10a9f8104e1ebb8fad044927ff130a0e2ce9131b

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
bit_shifts_runtime -118 ✅ -13.32% -141 ✅ -3.00%
eddsa -15,694 ✅ -24.28% -15,860 ✅ -24.10%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
merkle_insert 46 (-8) -14.81% 3,677 (-7) -0.19%
u128 611 (-44) -6.72% 4,604 (-61) -1.31%
bit_shifts_runtime 768 (-118) -13.32% 4,557 (-141) -3.00%
eddsa 48,937 (-15,694) -24.28% 49,957 (-15,860) -24.10%

@aakoshh aakoshh marked this pull request as ready for review November 25, 2024 16:15
@aakoshh aakoshh requested review from TomAFrench and a team November 25, 2024 16:21
Intrinsic::AssertConstant
| Intrinsic::StaticAssert
| Intrinsic::ApplyRangeConstraint
| Intrinsic::AsWitness => deduplicate_with_predicate,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between these four and the previous ones?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above // As long as the constraints are applied on the same inputs. this case also feels implied by "can_be_deduplicated"

Copy link
Contributor Author

@aakoshh aakoshh Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is in how much of the comments on the previous two don't apply on these. Basically followed the similar existing division in has_side_effects above. The ones in the first group are the ones I found appear in handle_instruction_side_effects in flatten_cfg, so these are tied to the predicate by the virtue of having their input being multiplied with it.

These others down here seemed related simply to applying assertions and constraints, which I assume work similar in that if you do the same constraint multiple times on the same thing then you can deduplicate. I hope this is true for AsWitness as well. I agree the comment above doesn't add any insight, I'll remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we want to deduplicate AsWitness. It is telling our ACIR gen to generate a new witness variable, which isn't exactly the same as an assertion. cc @TomAFrench

Copy link
Contributor Author

@aakoshh aakoshh Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought it might be enough to produce one witness for the same inputs, and hoped to clarify in the PR review with y’all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the purposes of std::as_witness, it doesn't require that distinct witnesses are created so I'd deduplicate these.

compiler/noirc_evaluator/src/ssa/ir/instruction.rs Outdated Show resolved Hide resolved
Intrinsic::AssertConstant
| Intrinsic::StaticAssert
| Intrinsic::ApplyRangeConstraint
| Intrinsic::AsWitness => deduplicate_with_predicate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above // As long as the constraints are applied on the same inputs. this case also feels implied by "can_be_deduplicated"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deduplicate intrinsics with side effects when they're under the same predicate
4 participants